opentelemetry: support building on non-x86 architectures#3869
Open
NormB wants to merge 1 commit intoOpenSIPS:masterfrom
Open
opentelemetry: support building on non-x86 architectures#3869NormB wants to merge 1 commit intoOpenSIPS:masterfrom
NormB wants to merge 1 commit intoOpenSIPS:masterfrom
Conversation
Building the new opentelemetry module on aarch64 (Jetson Thor, Debian
trixie, GCC 14) fails with:
pt.h:92:9: error: 'atomic_t' does not name a type
Looking into it, this turns out to be an issue on every non-x86
architecture, not just aarch64.
Root cause
opentelemetry.cpp is a C++ translation unit, but it includes C headers
from the core (pt.h, statistics.h, ...). Those headers reach atomic.h,
which defines atomic_t two different ways:
1. When HAVE_STDATOMIC is set, via the C11 keyword:
typedef _Atomic(unsigned long) atomic_t;
2. Otherwise, via a hand-rolled fallback:
typedef struct { volatile unsigned long counter; } atomic_t;
`_Atomic` is a C-only keyword and is not part of C++. The module
currently undef's HAVE_STDATOMIC at the top of the C++ source so that
atomic.h takes path (2). That works on __CPU_i386 and __CPU_x86_64,
which are the architectures where atomic.h provides the fallback
typedef. On aarch64, arm, ppc, mips, riscv, s390x, and others, atomic.h
has no fallback, so atomic_t ends up undefined and every header that
references it (pt.h, statistics.h) fails to compile.
Proposed approach
Rather than undef'ing HAVE_STDATOMIC — which forces a path that does
not exist for most architectures — another option is to keep
HAVE_STDATOMIC in whatever state the build system picked (it is set by
Makefile.defs or compiler auto-detection as appropriate) and shim the
C11 `_Atomic(T)` keyword to std::atomic<T> for this C++ translation
unit:
#ifdef __cplusplus
#include <atomic>
#define _Atomic(T) std::atomic<T>
#endif
atomic.h's typedef now resolves to `std::atomic<unsigned long> atomic_t`
under C++ on every architecture.
Why this should be ABI-safe
`std::atomic<T>` and C11 `_Atomic(T)` have identical memory layout on
every GCC/Clang target that supports both — it is a guarantee of the
standard library implementation (std::atomic<T> is `alignas(T) T` when
T is lock-free, which mirrors _Atomic(T)). The C headers only ever
treat atomic_t as an opaque type — they take its address and pass it
to helper functions like atomic_inc() / atomic_set(), and they never
read .counter directly from a C++ compile unit. So even though the C
side may see path (2)'s struct layout and the C++ side now sees
std::atomic<T>'s layout, the two interoperate correctly because neither
side reaches into the other's representation.
If the maintainers prefer a different approach (for example, extending
atomic.h with a fallback for the remaining architectures), we are happy
to adjust.
Tested on
- aarch64 (Jetson Thor, Debian trixie, GCC 14): the original failure
is gone; opentelemetry.so builds and loads into a running OpenSIPS;
spans export to an OTLP/HTTP collector; SIP attributes (sip.method,
sip.call_id, sip.ruri, net.peer.ip, ...) populate correctly on
spans produced by request and event routes.
- x86_64 (Debian trixie, GCC 14.2.0): no regression. The module still
builds and links cleanly against the Debian-packaged
opentelemetry-cpp 1.19.0 and libabsl-dev.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Building the new opentelemetry module on aarch64 (Jetson Thor, Debian trixie, GCC 14) fails with:
Looking into it, this turns out to be an issue on every non-x86 architecture, not just aarch64.
Root cause
opentelemetry.cpp is a C++ translation unit, but it includes C headers from the core (pt.h, statistics.h, ...). Those headers reach atomic.h, which defines atomic_t two different ways:
_Atomicis a C-only keyword and is not part of C++. The module currently undef's HAVE_STDATOMIC at the top of the C++ source so that atomic.h takes path (2). That works on __CPU_i386 and __CPU_x86_64, which are the architectures where atomic.h provides the fallback typedef. On aarch64, arm, ppc, mips, riscv, s390x, and others, atomic.h has no fallback, so atomic_t ends up undefined and every header that references it (pt.h, statistics.h) fails to compile.Proposed approach
Rather than undef'ing HAVE_STDATOMIC — which forces a path that does not exist for most architectures — another option is to keep HAVE_STDATOMIC in whatever state the build system picked (it is set by Makefile.defs or compiler auto-detection as appropriate) and shim the C11
_Atomic(T)keyword to std::atomic for this C++ translation unit:atomic.h's typedef now resolves to
std::atomic<unsigned long> atomic_tunder C++ on every architecture.Why this should be ABI-safe
std::atomic<T>and C11_Atomic(T)have identical memory layout on every GCC/Clang target that supports both — it is a guarantee of the standard library implementation (std::atomic isalignas(T) Twhen T is lock-free, which mirrors _Atomic(T)). The C headers only ever treat atomic_t as an opaque type — they take its address and pass it to helper functions like atomic_inc() / atomic_set(), and they never read .counter directly from a C++ compile unit. So even though the C side may see path (2)'s struct layout and the C++ side now sees std::atomic's layout, the two interoperate correctly because neither side reaches into the other's representation.If the maintainers prefer a different approach (for example, extending atomic.h with a fallback for the remaining architectures), we are happy to adjust.
Tested on
Summary
Details
Solution
Compatibility
Closing issues